Skip to content

sast: Phase 1 triage — fix Semgrep rule scope, dismiss 47 alerts#152

Merged
grove merged 5 commits intomainfrom
sast-review-1
Mar 10, 2026
Merged

sast: Phase 1 triage — fix Semgrep rule scope, dismiss 47 alerts#152
grove merged 5 commits intomainfrom
sast-review-1

Conversation

@grove
Copy link
Copy Markdown
Owner

@grove grove commented Mar 10, 2026

Summary

SAST Phases 1–3 + additional Phase 2 rule: alert triage, CI tuning, privilege-context rules, panic rule, and unsafe inventory.

Changes

Phase 1 — Alert triage

  • Tuned sql.security-definer.present to exclude plans/** / docs/** / **/*.md
  • Dismissed all 47 initial alerts with documented justifications

CI trigger tightening

  • Removed pull_request trigger from CodeQL and Semgrep workflows
  • Both still run on push-to-main and weekly Monday schedule

Phase 2 — Extension-specific Semgrep rules

  • sql.row-security.disabled: flag SET LOCAL row_security = off
  • sql.set-role.present: flag SET ROLE / RESET ROLE
  • rust.panic-in-sql-path: flag .unwrap(), .expect(), panic!() in src/** — panics abort the PostgreSQL backend process; ~37 hits triaged, mostly known-safe idioms
  • sql.security-definer.present message updated with explicit SET search_path guidance
  • All rules advisory (WARNING/INFO, SARIF upload only)

Phase 3 — Unsafe growth controls

  • scripts/unsafe_inventory.sh: per-file unsafe { counter
  • .unsafe-baseline: committed baseline (1309 blocks across 6 files)
  • .github/work- .github/work- .github/work- .github/work- .github/work- .github/work- .githuben- .github/work- .github/work- .github/work- .github/worunt- .github/work- .gitn | |-------|------|------|----------| | 2 | CodeQL | CWE-312 cleartext logging/storage | used in tests — synthetic salary dat| 2 | CodeQL | CWE-312 cleartext logging/storag-definer.present | false positive — markdown design docs; rule fixed |
    | 32 | Semgrep | spi.run/query.dynamic-format | false positive — all pre-quoted identifiers, OIDs, or internal names |

Semgrep rules (6 total — all advisory)

Rule Purpose
rust.spi.run.dynamic-format Dynamic SQL to Spi::run
rust.spi.query.dynamic-format Dynamic SQL to Spi::get_*
sql.security-definer.present SECURITY DEFINER review + search_path guidance
sql.row-security.disabled SET LOCAL row_security = off
sql.set-role.present SET ROLE / RESET ROLE
rust.panic-in-sql-path .unwrap() / .expect() / panic!() in src/**

What's next (Phase 4)

Promote high-confidence rules to blocking after v0.3.0 RLS/privilege-hardeningPromote high-confidence rules to -context rules.

grove added 2 commits March 10, 2026 19:32
The generic-language security-definer rule was matching SECURITY DEFINER
occurrences in plans/sql/*.md design documents because Semgrep's path
matching treats 'sql/**' as a substring match that covers 'plans/sql/'.

Add explicit excludes for **/*.md, plans/**, and docs/** so the rule only
fires on actual SQL and Rust source files.

Also dismiss all 47 open Security tab alerts:
- 2 CodeQL CWE-312 (used in tests: synthetic salary data in e2e fixtures)
- 13 Semgrep security-definer (false positive: docs/plans markdown files)
- 32 Semgrep spi.run/query.dynamic-format (false positive: triaged
  2026-03-10; all interpolated values are pre-quoted catalog identifiers,
  OIDs, or internal temp table names — none reachable from user input)
- Mark Phase 0 as complete with actual CI outcomes (CodeQL 0 findings,
  deny clean, 47 Semgrep alerts triaged)
- Mark Phase 1 as complete with list of completed tasks
- Update Implementation Checklist: restructure into Phase 0 done /
  Phase 1 done / Phase 2 next / Phase 3 pending
- Update Phase completion criteria table with checkmarks
- Add Triage Log section documenting all 47 alert dismissals:
  - 2 CodeQL CWE-312 false positives (salary in test fixtures)
  - 13 Semgrep security-definer hits in plan/doc markdown files
  - 32 Semgrep SPI dynamic-format hits (all pre-quoted, not user input)
- Update Table of Contents
- Replace stale Recommendation text with Phase 2 roadmap
@grove grove requested a review from BaardBouvet as a code owner March 10, 2026 18:35
grove added 3 commits March 10, 2026 19:40
Phase 2 — Extension-specific privilege-context Semgrep rules:
- sql.row-security.disabled: flag SET LOCAL row_security = off
- sql.set-role.present: flag SET ROLE / RESET ROLE
- Update sql.security-definer.present message with SET search_path guidance
  (search-path hijacking risk explicitly called out for reviewers)
- All three rules advisory (WARNING/INFO), scoped to src/** + sql/**,
  excluding *.md / plans/** / docs/**

Phase 3 — Unsafe block inventory:
- scripts/unsafe_inventory.sh: per-file grep-based unsafe { counter
  with --report-only and --update modes; writes GITHUB_STEP_SUMMARY in CI
- .unsafe-baseline: committed baseline (1309 blocks across 6 files)
- .github/workflows/unsafe-inventory.yml: advisory CI workflow that runs
  on PRs touching src/**; fails if any file exceeds its baseline count
- just unsafe-inventory recipe for local use

Docs:
- plans/testing/PLAN_SAST.md: Phase 2 and 3 marked complete
- CHANGELOG.md: Phase 2+3 entries added
- ROADMAP.md: SAST section added tracking all S1-S8 items
.unwrap(), .expect(), and panic!() abort the PostgreSQL backend process
if reached from a SQL-callable function. The new advisory rule surfaces
all ~37 existing callsites in src/** for triage.

Known patterns in the existing hits:
- expect("unreachable after error!()") — valid pgrx idiom post-error!()
- expect() inside #[cfg(test)] mod tests blocks (recursive_cte.rs:3270+)
- a handful of genuine candidates in api.rs and parser.rs

All hits are advisory; none block CI.
@grove grove merged commit 1bb49ff into main Mar 10, 2026
15 checks passed
@grove grove deleted the sast-review-1 branch March 10, 2026 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant